Enabling TLS 1.3 on Windows #732
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #732 +/- ##
=======================================
Coverage 78.74% 78.74%
=======================================
Files 30 30
Lines 6385 6385
=======================================
Hits 5028 5028
Misses 1357 1357 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This reverts commit 318f7e5.
| @@ -104,7 +104,18 @@ add_net_test_case(cleanup_before_connect_or_timeout_doesnt_explode) | |||
| endif() | |||
|
|
|||
| if(WIN32) | |||
| set(WIN_VERSION ${CMAKE_SYSTEM_VERSION}) | |||
There was a problem hiding this comment.
This check might no longer works after cmake 3.27, as CMAKE_SYSTEM_VERSION will be disabled after cmake 3.27. https://cmake.org/cmake/help/latest/variable/CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION.html
Probably added a comment that this check might not work after 3.27.
There was a problem hiding this comment.
Nice catch! So, it seems CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION should be used here instead
There was a problem hiding this comment.
Though, thinking more about it:
CMAKE_SYSTEM_VERSION is the version of the OS where tests are running.
CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION is the version of Windows SDK that is used.
TLS 1.3 availability is determined by both the Windows OS version and Windows SDK version.
We can add extra check for CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION . But considering that our CI uses modern Windows SDKs, it might be overkill.
There was a problem hiding this comment.
It is possible that CMAKE_SYSTEM_VERSION is overwrite by build configuration. CMAKE_SYSTEM_VERSION could be used to set to a target build system version instead of the OS version it actually running on the host. Probably use CMAKE_HOST_SYSTEM_VERSION if we would like to check the actual OS version?
There was a problem hiding this comment.
You're right, CMAKE_HOST_SYSTEM_VERSION is better suited
Issue #, if available:
Another attempt to support TLS 1.3 on Windows platform.
Previous PR #676 didn't handle older Windows SDKs, so we had to revert it.
Description of changes:
Support TLS 1.3 on Windows when mTLS is used.
Other changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.